Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#9706: Allow time range filtering for Attribute Table quick filter #9743

Merged
merged 19 commits into from
Jan 22, 2024

Conversation

mahmoudadel54
Copy link
Contributor

Description

  • adding filter operators dropdown list in attribute table that includes range operator for time/date attributes
  • creating a custom picker for range
  • write unit tests
  • Edit in style

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Issue

#9706

What is the current behavior?
#9706

What is the new behavior?
Different filter operators dropdown is added to the attribute table columns including range time/date

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

…quick filter and allow additional filter operators for other attributes

Description:
- adding filter operators dropdown list in attribute table that includes range operator for time/date attributes
- creating a custom picker for range
- write unit tests
- Edit in style
Copy link
Contributor

@MV88 MV88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functionality works well, but there a few things to change and some inconsistencies/bugs to handle:

  • operator must match
    image
  • cursor style
    image
  • invalid range
    image
  • one single icon was requested
    image
  • little style issue
    image
  • improve focus style on time picker
    image
  • improve style on operator
    image
  • inconsistent value after wsitching back from between operator ><
    image
  • single datetime field is not using new component and must use it

tooltipMsgId: PropTypes.string,
operator: PropTypes.string,
type: PropTypes.string,
isWithinAttrTbl: PropTypes.bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think that this should be removed in the future since also attribute table in dashboard should inherit timerange changes

Copy link
Contributor

@MV88 MV88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some extra points

@tdipisa tdipisa added this to the 2024.01.00 milestone Nov 27, 2023
Desciption:
- remove expression from filter inputs and make validation for the inputs
- Add cursor style to 'Start' & 'End' tabs
- Add validation for  ranges
- Replace 2 icons with one icon for date-time filter
- Fix style issues
- Fixed 'Start', 'End' tabs in scroll
- improve style of operator
- reset date/time if user change operator from range operator to another
@mahmoudadel54 mahmoudadel54 requested a review from MV88 December 1, 2023 09:06
Copy link
Contributor

@MV88 MV88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • this is not done "one single icon was requested" as it was requested in the issue here
    image
  • calendar is not fully visible, can you add at leas a scrollbar if you can't display as drop up?
    image

the rest seems ok a part some inline comment

@mahmoudadel54
Copy link
Contributor Author

mahmoudadel54 commented Dec 14, 2023

  • this is not done "one single icon was requested" as it was requested in the issue here

I don't know how can I get the attached icon, I searched for the icons in react-widget lib version 3.5.0 but I didn't find the required one
@MV88

  • calendar is not fully visible, can you add at leas a scrollbar if you can't display as drop up?

I have tried much time to use drop up but there is a big css issue I couldn't resolve it and during trying handle this point, it reflects on other css for the attribute table
I will try the 1st suggestion of making scroll

@mahmoudadel54
Copy link
Contributor Author

@MV88
For adding scroll in dates picker, this is a demo demonstrating implementation of it. Is it acceptable ?

scroll.in.dates.pickers.mp4

@MV88
Copy link
Contributor

MV88 commented Dec 14, 2023

  • this is not done "one single icon was requested" as it was requested in the issue here

I don't know how can I get the attached icon, I searched for the icons in react-widget lib version 3.5.0 but I didn't find the required one @MV88

I'm sorry maybe you haven't done it before.
You have to create a new one

  • calendar is not fully visible, can you add at leas a scrollbar if you can't display as drop up?

I have tried much time to use drop up but there is a big css issue I couldn't resolve it and during trying handle this point, it reflects on other css for the attribute table I will try the 1st suggestion of making scroll

ok i'll try to give a better look, but maybe @allyoucanmap has a suggestion on this point

@MV88
Copy link
Contributor

MV88 commented Dec 14, 2023

@MV88 For adding scroll in dates picker, this is a demo demonstrating implementation of it. Is it acceptable ?

scroll.in.dates.pickers.mp4

at least you can view almost all data, there is still some part not well selectable

@allyoucanmap
Copy link
Contributor

@mahmoudadel54 @MV88 here the icon
date-time.zip

@MV88 MV88 self-requested a review December 15, 2023 09:04
Copy link
Contributor

@MV88 MV88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mahmoudadel54 hi

things to do here

  • use new icon made by @allyoucanmap
  • solve conflict
  • try to fix at least the bottom part of the datepicker
    image

mahmoudadel54 and others added 4 commits December 15, 2023 11:27
…quick filter

* Description:
- Add date-time icon and use it
- replcae inline if
- using omit instead of reduce
- fix some style issues in date/time pickers in range component
…quick filter

* Description:
- Fix FE test failure
@mahmoudadel54 mahmoudadel54 requested a review from MV88 December 18, 2023 06:29
Description:
- Improve UI of pickers by using popover component in dateTime/Date/Hours pickers
- Enable update popover position in case of scroll the container conponent
-  write unit tests due to changes
- Rename range date-time picker by 'RangedDateTimePicker'
@mahmoudadel54
Copy link
Contributor Author

@mahmoudadel54 hi
things to do here

  • use new icon made by @allyoucanmap
  • solve conflict
  • try to fix at least the bottom part of the datepicker
    image

@tdipisa the third point here is not resolved and i tried locally to find an alternative fix but it could require some refactor effort and for the moment i'm going to put this in blocked.

i'll also push some changes to a separate branch that will produce a similar result but which is incomplete and not fully working image

The refactor parts are

  • there will be three new components:

    • a DateTimePicker that will allow user to select a date only, a time only, a date and time
    • a RangedDateTimePicker that will reuse the previous component to show the ranged mode
    • a new Time.jsx component that will allow to type or select an hour
    • otherwise we can maintain the old DateTimePicker and CustomDateTimePickerWithRange and refactor them later on
  • for the time picker we can simply use a combobox and not current component,

    • we can keep the selected value with background as primary or the default selected bg color we have, e.g
      image
    • create a new component for the moment called Time in web\client\components\misc\datetimepicker
    • quickly check if this is applicable to the one in queryform
    • we have then check if clicks in and outside the container behaves correctly
    • we need to maintain the possibility to edit the value manually once selected from the combobox
  • in the case of datetime picker both picker must be visible from the start, so the overall style must be refined or adapted (like reducing paddings etc making it more compact than what is in the image above)

  • this means to avoid to show the vertical scrollbar as well

  • when user clicks on time picker it should expand at the top if if space is not available

  • the date time picker will hide if user clicks outside the related container of the datetime picker

Hi @MV88
I have tried to use Combobox component as you mentioned but it didn't work as expected so I tried to use popover component as in your POC branch and this is a demo demonstrating the UI improvements in pickers
Also I have fixed the issue of fixing the popover even if user makes some scroll in the UI like here if user opens date picker and wants to scroll into horizontal direction [it is in demo as well]

UI.improvements.for.datetime.pickers.mp4

Copy link
Contributor

@MV88 MV88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have noticed these

  • image
  • reopening the combobox the selected hour is not visible
    image
  • editing year manually in the input field is is not aligned to the calendar
    image

please better check how this tools can be used to spot these problems

Description:
- Handle highlight selected hours component
- Fix not alignment in dateTime picker
- Fix not alignment selected date in calendar for date/ dateTime pickers
- Minimize calendar row height to improve showing date picker UI
- Create a util function  'getLocalTimePart' to get time part for local time
@mahmoudadel54 mahmoudadel54 requested a review from MV88 January 16, 2024 09:38
Copy link
Contributor

@MV88 MV88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • placeholder for time should be type a time to filter...
    image
  • when passing from date time ranged to a single datetime app crashed
    image
datetime.range.crash.mp4
  • also you can enter 70 seconds and it's not parsed. but i think we can improve this in another pr

Description:
- Add placeholder for time picker
- Add translations for the added time placeholder
- Fix crash app bug due to entering invalid date in input texts of date/time Pickers
@mahmoudadel54 mahmoudadel54 requested a review from MV88 January 18, 2024 16:39
Copy link
Contributor

@MV88 MV88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks ok. a few changes here

web/client/translations/data.it-IT.json Outdated Show resolved Hide resolved
web/client/translations/data.it-IT.json Outdated Show resolved Hide resolved
Copy link
Contributor

@MV88 MV88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • there is an issue with the popover size when mode is changed in the calendar
popover.issue.in.height.change.of.content.container.mp4

@dsuren1
Copy link
Contributor

dsuren1 commented Jan 22, 2024

Resizing the height of the popup to display calendar content doesn't look pleasing as the varying size makes it convoluted for user interactions. Hence a fixed height is used instead as shown below.

Date filter

Screen.Recording.2024-01-22.at.6.57.56.PM.mov

Date-time filter

Screen.Recording.2024-01-22.at.7.02.33.PM.mov

@dsuren1 dsuren1 requested a review from MV88 January 22, 2024 14:13
@tdipisa
Copy link
Member

tdipisa commented Jan 22, 2024

@MV88 can you please proceed asap with the review?

@MV88 MV88 merged commit 93ea6ea into geosolutions-it:master Jan 22, 2024
6 checks passed
@MV88
Copy link
Contributor

MV88 commented Jan 22, 2024

@ElenaGallo please test it in DEV using a date, time and date time layer like this map https://dev-mapstore.geosolutionsgroup.com/mapstore/#/viewer/28584

@ElenaGallo
Copy link
Contributor

ElenaGallo commented Jan 22, 2024

Hi @dsuren1, I only notice that some filters, in edit mode, are enabled
1

How to reproduce

  • Open the attribute table
  • Click on Edit mode

@tdipisa
Copy link
Member

tdipisa commented Jan 22, 2024

@ElenaGallo please open a new issue for the above and assign it to refer to @dsuren1.
@dsuren1 that should be a quick fix to do. Thank you.

@dsuren1
Copy link
Contributor

dsuren1 commented Jan 23, 2024

@ElenaGallo
image
I believe this is an expected behavior. Kindly check this map in QA

The only problem I see is that when the field is disabled, the operator dropdown should be disabled too, as some are not, from the screenshot you shared. Additionally, the time fields are not disabled (in edit mode) and it should, which is an existing problem in QA. Kindly report this in the new issue. Thanks!

@tdipisa
Copy link
Member

tdipisa commented Jan 23, 2024

@dsuren1 that's effectively the reason why I've asked to open a new issue. Thank you for clarifying this anyway.

@ElenaGallo
Copy link
Contributor

@dsuren1 @tdipisa new issue here: #9901

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow time range filtering for Attribute Table quick filter
6 participants